Fix #9050: Allow multidenotations with same signature #9063
Conversation
odersky
force-pushed
the
fix-#9050
branch
2 times, most recently
from
d948982 to
70bf21b
Compare
|
test performance please |
|
performance test scheduled: 1 job(s) in queue, 0 running. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/9063/ to see the changes. Benchmarks is based on merging with master (2116281) |
|
test performance please |
|
test performance please |
|
performance test scheduled: 1 job(s) in queue, 1 running. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/9063/ to see the changes. Benchmarks is based on merging with master (5e9d927) |
|
test performance please |
|
performance test scheduled: 1 job(s) in queue, 1 running. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/9063/ to see the changes. Benchmarks is based on merging with master (af37d64) |
|
test performance please |
|
performance test scheduled: 2 job(s) in queue, 1 running. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/9063/ to see the changes. Benchmarks is based on merging with master (0540e5e) |
Instead of issuing a merge error, allow multi-denotations with alternatives that have the same signature.
Test only for methods. Others cannot get signature clashes through asSeenFrom. That is, if they clash after asSeenFrom, they already clash before.
Wait until overloading resolution instead.
|
performance test scheduled: 1 job(s) in queue, 1 running. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/9063/ to see the changes. Benchmarks is based on merging with master (1ef1765) |
|
test performance please |
|
performance test scheduled: 2 job(s) in queue, 1 running. |
| val linScore = linearScore(sym1.owner, sym2.owner) | ||
| if linScore != 0 then linScore | ||
| else | ||
| val boundary1 = accessBoundary(sym1) | ||
| val boundary2 = accessBoundary(sym2) | ||
| if boundary1.isProperlyContainedIn(boundary2) then -1 | ||
| else if boundary2.isProperlyContainedIn(boundary1) then 1 | ||
| else if sym1.is(Bridge) && !sym2.is(Bridge) then -2 | ||
| else if sym2.is(Bridge) && !sym1.is(Bridge) then 2 | ||
| else if sym2.is(Method) && !sym1.is(Method) then -2 | ||
| else if sym1.is(Method) && !sym2.is(Method) then 2 |
There was a problem hiding this comment.
How is the ordering of these checks determined? I would have expected them to be ordered to preserve monotonicity when possible: e.g. given that a bridge vs a non-bridge gives a score of -2, I would not expect that score to decrease because of the relative access boundary of that bridge. Is there some other property than monotonicity that can guide us here?
There was a problem hiding this comment.
(it's also worth noting that as of dotty-staging@32f9deb we should never encounter any bridge symbol until Erasure, so that particular check might not be needed anymore)
There was a problem hiding this comment.
It models the ordering that we had previously.
|
test performance please |
|
@liufengyun Could you check the benchmark application? It never came back from the previous request. |
|
I'll have a look at the problem of the previous request. The bot does not respond just now because it stopped early this morning due to a network error (lampepfl/bench#213), and now it's back to be working again. |
|
performance test scheduled: 2 job(s) in queue, 1 running. |
|
The problem is spotted (lampepfl/bench#214), will fix ASAP and run the test. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/9063/ to see the changes. Benchmarks is based on merging with master (5fce8e6) |
|
Thanks for getting the benchmark running, Fengyun! Looking at the latest number it seems it's fast enough without the dubious optimization with the |
| * pick the associated denotation. | ||
| * 3. Otherwise, if the two infos can be combined with `infoMeet`, pick that as | ||
| * result info, and pick the symbol that scores higher as result symbol, | ||
| * or pick `sym2` as a tie breaker. The picked info and symbol are combined |
There was a problem hiding this comment.
Why sym2 and not sym1 as the tie breaker?
There was a problem hiding this comment.
It was sym2 before. I now went through the callers, but I can see no reason why it should be. Single denotations are merged with the left operand first in linearization order. So it makes some sense to keep it that way (maybe?, or it makes no difference at all, I am not sure).
community-build/test/scala/dotty/communitybuild/CommunityBuildTest.scala
Show resolved
Hide resolved
| @@ -122,7 +123,7 @@ Standard-Section: "ASTs" TopLevelStat* | |||
| TERMREFsymbol sym_ASTRef qual_Type -- A reference `qual.sym` to a local member with prefix `qual` | |||
| TERMREFpkg fullyQualified_NameRef -- A reference to a package member with given fully qualified name | |||
| TERMREF possiblySigned_NameRef qual_Type -- A reference `qual.name` to a non-local member | |||
| TERMREFin Length possiblySigned_NameRef qual_Type namespace_Type -- A reference `qual.name` to a non-local member that's private in `namespace` | |||
| TERMREFin Length possiblySigned_NameRef qual_Type owner_Type -- A reference `qual.name` referring to a non-local symbol declared in owner that has the given signature (see note below) | |||
There was a problem hiding this comment.
Should the documentation for TYPEREFin be updated in a similar way?
There was a problem hiding this comment.
No since TypeRefs never take a signature
Instead of issuing a merge error, allow multi-denotations with alternatives that have the same
signature.